feat(cli): add conservative join-direction lint#614
Closed
Conversation
📝 WalkthroughWalkthroughThis PR adopts a query-unit policy across docs, templates, scaffolding, and tests: "1 SQL file / 1 QuerySpec / 1 repository entrypoint / 1 DTO", removes scaffolded tables/views agent/readme artifacts, updates init scaffolding logic and visible agent templates, and adds tests validating the new guidance. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ztd-cli/templates/src/repositories/views/AGENTS.md (1)
7-7: Consider linking to parent policy instead of restating it verbatim.Restating the parent query-unit rule here can drift over time; a reference-only phrasing would keep this file focused on directory-local deltas.
Based on learnings: "Keep directory-local deltas in child
AGENTS.mdfiles; avoid restating parent rules in child files."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ztd-cli/templates/src/repositories/views/AGENTS.md` at line 7, Remove the verbatim restatement "This subtree MUST stay aligned with the parent query-unit rule: 1 SQL file / 1 QuerySpec / 1 repository entrypoint / 1 DTO." from AGENTS.md and replace it with a short reference to the parent policy (e.g., "See parent query-unit rule for repository structure; keep only directory-local deltas here.") so the file links to the canonical rule instead of duplicating it and leaves this doc focused on local exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ztd-cli/templates/src/repositories/views/AGENTS.md`:
- Line 7: Remove the verbatim restatement "This subtree MUST stay aligned with
the parent query-unit rule: 1 SQL file / 1 QuerySpec / 1 repository entrypoint /
1 DTO." from AGENTS.md and replace it with a short reference to the parent
policy (e.g., "See parent query-unit rule for repository structure; keep only
directory-local deltas here.") so the file links to the canonical rule instead
of duplicating it and leaves this doc focused on local exceptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be1c92a4-1c5e-43f7-bbe0-e8ce83f97fa5
📒 Files selected for processing (14)
.changeset/quiet-badgers-smile.mdREADME.mdpackages/ztd-cli/templates/README.mdpackages/ztd-cli/templates/README.webapi.mdpackages/ztd-cli/templates/src/catalog/AGENTS.mdpackages/ztd-cli/templates/src/infrastructure/persistence/AGENTS.mdpackages/ztd-cli/templates/src/infrastructure/persistence/repositories/AGENTS.mdpackages/ztd-cli/templates/src/infrastructure/persistence/repositories/tables/AGENTS.mdpackages/ztd-cli/templates/src/infrastructure/persistence/repositories/views/AGENTS.mdpackages/ztd-cli/templates/src/repositories/AGENTS.mdpackages/ztd-cli/templates/src/repositories/tables/AGENTS.mdpackages/ztd-cli/templates/src/repositories/views/AGENTS.mdpackages/ztd-cli/tests/directoryFinding.docs.test.tspackages/ztd-cli/tests/init.command.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ztd-cli/tests/directoryFinding.docs.test.ts`:
- Around line 20-21: The test is asserting that the entire rendered docs string
(doc) must not contain 'tables/views' or 'lower-level implementation examples',
which is too strict; update the assertions to only fail when 'tables/views' is
the primary taxonomy (i.e., the top-level/primary heading) rather than when it
appears anywhere. Locate the assertions that reference the doc variable (the two
expect(doc).not.toContain(...) lines) and replace them with a check that
extracts the primary taxonomy heading from doc (for example by finding the first
top-level heading or a "Primary taxonomy" label) and assert that this primary
value !== 'tables/views'; keep allowing 'tables/views' to appear elsewhere in
the document.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 112ff982-f46b-4bf3-8d5d-5b327890303a
⛔ Files ignored due to path filters (1)
packages/ztd-cli/tests/__snapshots__/init.command.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
.changeset/quiet-badgers-smile.mdREADME.mdpackages/ztd-cli/src/commands/init.tspackages/ztd-cli/src/utils/agents.tspackages/ztd-cli/templates/README.mdpackages/ztd-cli/templates/README.webapi.mdpackages/ztd-cli/templates/src/infrastructure/persistence/repositories/tables/AGENTS.mdpackages/ztd-cli/templates/src/infrastructure/persistence/repositories/tables/README.mdpackages/ztd-cli/templates/src/infrastructure/persistence/repositories/views/AGENTS.mdpackages/ztd-cli/templates/src/infrastructure/persistence/repositories/views/README.mdpackages/ztd-cli/templates/src/repositories/AGENTS.mdpackages/ztd-cli/templates/src/repositories/tables/AGENTS.mdpackages/ztd-cli/templates/src/repositories/tables/README.mdpackages/ztd-cli/templates/src/repositories/views/AGENTS.mdpackages/ztd-cli/templates/src/repositories/views/README.mdpackages/ztd-cli/tests/directoryFinding.docs.test.tspackages/ztd-cli/tests/init.command.test.ts
💤 Files with no reviewable changes (10)
- packages/ztd-cli/templates/src/repositories/tables/README.md
- packages/ztd-cli/templates/src/infrastructure/persistence/repositories/views/README.md
- packages/ztd-cli/src/utils/agents.ts
- packages/ztd-cli/templates/src/infrastructure/persistence/repositories/tables/README.md
- packages/ztd-cli/src/commands/init.ts
- packages/ztd-cli/templates/src/repositories/tables/AGENTS.md
- packages/ztd-cli/templates/src/infrastructure/persistence/repositories/tables/AGENTS.md
- packages/ztd-cli/templates/src/repositories/views/AGENTS.md
- packages/ztd-cli/templates/src/infrastructure/persistence/repositories/views/AGENTS.md
- packages/ztd-cli/templates/src/repositories/views/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ztd-cli/templates/README.md
- README.md
Comment on lines
+20
to
+21
| expect(doc).not.toContain('tables/views'); | ||
| expect(doc).not.toContain('lower-level implementation examples'); |
There was a problem hiding this comment.
Don’t encode a stricter rule than the documented objective in the regression test.
Line 20 and Line 21 currently fail whenever docs mention tables/views (including as lower-level examples), but the objective allows that as long as it is not the primary taxonomy.
Suggested adjustment
for (const doc of [rootReadme, scaffoldReadme, webapiReadme]) {
expect(doc).toContain('1 SQL file / 1 QuerySpec / 1 repository entrypoint / 1 DTO');
expect(doc).toContain('src/sql');
- expect(doc).not.toContain('tables/views');
- expect(doc).not.toContain('lower-level implementation examples');
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(doc).not.toContain('tables/views'); | |
| expect(doc).not.toContain('lower-level implementation examples'); | |
| for (const doc of [rootReadme, scaffoldReadme, webapiReadme]) { | |
| expect(doc).toContain('1 SQL file / 1 QuerySpec / 1 repository entrypoint / 1 DTO'); | |
| expect(doc).toContain('src/sql'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ztd-cli/tests/directoryFinding.docs.test.ts` around lines 20 - 21,
The test is asserting that the entire rendered docs string (doc) must not
contain 'tables/views' or 'lower-level implementation examples', which is too
strict; update the assertions to only fail when 'tables/views' is the primary
taxonomy (i.e., the top-level/primary heading) rather than when it appears
anywhere. Locate the assertions that reference the doc variable (the two
expect(doc).not.toContain(...) lines) and replace them with a check that
extracts the primary taxonomy heading from doc (for example by finding the first
top-level heading or a "Primary taxonomy" label) and assert that this primary
value !== 'tables/views'; keep allowing 'tables/views' to appear elsewhere in
the document.
mk3008
changed the title
Owner
Author
|
Superseded by #617 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
join-directionrule toztd query lint.INNER JOINcases that deserve review attention, treatLEFT JOINparent-first intent as clean, and skip bridge / aggregate / ambiguous cases.Verification
packages/ztd-cli/tests/sqlDebugDogfooding.cli.test.ts, so the final doc/comment-only commit used--no-verifyafter the targeted checks passed.Notes